-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add codespell: config + action (to detect new typos) and make it fix a good number of them #11195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: readthedocs/api/v2/views/integrations.py
Did you find this useful? React with a 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR 👍🏼
I didn't take a full review, but I noted a few issues that I see will generate some conflicts. In particular, those that change Python code and require some manual interaction (e.g. a Django migration to update the help_text
)
I'm not opposed to this work, but I find it hard to prioritize by the core team at this point.
We use pre-commit for all other linting checks. I feel if we implement this it should be at pre-commit, not GHA. |
Hmm, interesting, my script should've detected presence of pre-commit config... Will have a look wherever get a chance. You do have ci job running the pre-commit I assume? (I don't see report from pre-commit service) |
that explains it: ❯ ls -ld .pre-commit-config.yaml
lrwxrwxrwx 1 yoh yoh 29 Mar 7 18:00 .pre-commit-config.yaml -> common/pre-commit-config.yaml
❯ cat .pre-commit-config.yaml
cat: .pre-commit-config.yaml: No such file or directory
❯ git submodule
-4af0fffd2cbeeb40f0a71b875beb99d6dc88a355 common Let's do the submodules dance now... |
51b4bb1
to
ef04ef4
Compare
ef04ef4
to
120e693
Compare
This PR is great! Thanks for opening it! I'd like to move forward with it. I've already reviewed readthedocs/common#212 We need to make some adjustments here and we can move forward 👍🏼 . @yarikoptic are you able to continue with this work?
Let's exclude the changelog for now. It has a lot of noise.
We should implement this, if it's not already. |
I am able as soon as there is cycles on your end so PR doesn't amass conflicts to address again. |
120e693
to
40091fd
Compare
Documentation build overviewFiles changed
Show files (5) | 5 modified | 0 added | 0 deleted
|
Documentation build overviewFiles changed
Show files (1) | 1 modified | 0 added | 0 deleted
|
FWIW, I have rebased and redone codespell fixing (automated and interactive). Let's see if no side effects. Please review the diff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I suppose there may be a few test failing because there are some words that shouldn't return search results.
readthedocs/search/tests/test_api.py
Outdated
@@ -526,7 +526,7 @@ def test_search_advanced_query_detection(self, api_client): | |||
search_params = { | |||
"project": project.slug, | |||
"version": version.slug, | |||
"q": "indx", | |||
"q": "index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was probably an invalid word on purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we need to annotate this line to be skipped and ok to contain indx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced it with explicit "abracadabra"
, or it does have to be "indx"
? (let's see if tests feel better)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is failing. I think it needs to be indx
probably for fuzzy search testing. I would leave as it was to avoid this type of issues and probably mark that line to be skipped 👍🏼
…nteractively === Do not change lines below === { "chain": [], "cmd": "codespell -w -i 3 -C 4", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^
…os automagically === Do not change lines below === { "chain": [], "cmd": "codespell -w", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^
40091fd
to
c5202bd
Compare
This is indeed a typo that I made when I created the model's field |
[codespell] | ||
# Ref: https://github.com/codespell-project/codespell#using-a-config-file | ||
skip = .git,*.svg,locale,package-lock.json,*.css,*.min.*,vendor,*.ai,setup.cfg,migrations,CHANGELOG.rst,common | ||
check-hidden = true | ||
# some names and abbreviations and very long lines (minimized?) | ||
# TODO: fixup help_text in readthedocs/builds/models.py : "to perfom an" | ||
ignore-regex = \b(Manuel|DED|Wile E. Coyote|Couldn\\u2019t|to perfom an)\b|.{300,}|"pyton\b|\|(ative|ment)\||"Hel" will match\b|ative: ''|help_text *=.* | ||
# TODO: fix syntaxt -- would require transition? | ||
ignore-words-list = fo,te,astroid,requestor,syntaxt,ore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to put all this configuration on a specific codespell file? Like .codespell.cfg
or similar?
We will need to put this file into common/
repository (readthedocs/common#212) because we want to share this configuration across multiple repositories, eg. https://github.com/readthedocs/ext-theme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems .codespellrc
https://github.com/codespell-project/codespell#using-a-config-file
We want that file to be in common/
and symlinked from here 👍🏼
codespell was used occasionally on some files, so not a new tool here. But now it would guard RTD from having typos introduced to begin with.
Note that some fixes could affect (fix) functionality.
There is an odd "variable"
syntaxt
which I didn't fix since seems would require transition and smells like it is on purpose:❯ git grep syntaxt -- readthedocs/ readthedocs/projects/migrations/0106_add_addons_config.py: ("syntaxt", models.CharField(max_length=128)), readthedocs/projects/models.py: syntaxt = models.CharField(max_length=128) readthedocs/projects/tasks/builds.py: grab them by using glob syntaxt between other files that could be garbage.
or was it intended to be
syntax
?TODOs
📚 Documentation previews 📚
docs
): https://docs--11195.org.readthedocs.build/en/11195/dev
): https://dev--11195.org.readthedocs.build/en/11195/